core: sorted reorg insertion order for proper head header updating#15941
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Currently when we're doing reorgs, the latest block is correctly updated to whatever it truly is, but the latest header lags behind until the next block is imported.
A repro log from the included test (plotting
CurrentHeader, theCurrentBlockis correct):This caused issues on the light clients where they got left behind a few blocks (seemingly not seeing that new content is available), or specifically on Rinkeby we've seen the faucet import blocks 2-by-2, not sequentially, which we couldn't explain until now.
The bug was a combination of lazy code and a subsequent refactor:
core/BlockChainhas aninsert()method, which originally updated our chain head to whatever it was given (e.g.insert(block 2)would update head to2, irrelevant what block we're at currently). The reorg code assumed this behavior blindly, and when inserting a chain of newly-became-canonical blocks, it inserted them in reverse order for some arbitrary lazy reason "Order does not matter. Last block will be written in ImportChain itself which creates the new head properly" and then topped the latest block on top. Internally reorging a new chain with blocks3-4-5meant that we set head to5-4-3-5, arriving to a correct head at the end. This worked correctly in its current form although during execution the internal state could be considered invalid.insertwas modified to only change thelatest headerif the block being inserted is not canonical yet.The combo effect of the above two ideas is that calling
inserton5-4-3-5will set the latest header to5-4-3, but will not set it to5again, since it sees that5is already a canonical header. The fix is kind of obvious: don't reorg in a weird reverse order, rather insert the blocks in their proper sequence. This would result in a reorg callinginserton3-4-5-5. This will properly set the latest block to3-4-5-5and will set the latest header to3-4-5.Note, I'm unsure about the design idea behind inserting the last block twice, and that should definitely warrant exploration from a performance perspective, but that's out of the scope of this PR and I'l like to have this PR be small and quickly fix a bug. We can explore performance issues in a followup PR.